fix: avoid panic in date_bin compute_distance near i64::MIN#22408
fix: avoid panic in date_bin compute_distance near i64::MIN#22408SAY-5 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
@SAY-5
Thanks for working on this fix. I think there’s still one overflow edge case that can panic instead of returning the expected error/NULL behavior, and I left one suggestion about expanding coverage to the SQL path as well.
| fn compute_distance(time_diff: i64, stride: i64) -> i64 { | ||
| let time_delta = time_diff - (time_diff % stride); | ||
| fn compute_distance(time_diff: i64, stride: i64) -> Result<i64> { | ||
| let remainder = time_diff % stride; |
There was a problem hiding this comment.
Nice catch making this path fallible overall. I think there’s still one unchecked overflow case left here though.
time_diff % stride can still panic when time_diff == i64::MIN and stride == -1. Right now date_bin_impl only rejects stride == 0, so a negative interval can still reach this code path and panic instead of returning the normal error/NULL behavior this fix is aiming for.
Could we switch this to checked_rem here, or alternatively reject non-positive strides before calling compute_distance?
It would also be great to add a regression test covering the i64::MIN, -1 case.
| } | ||
|
|
||
| #[test] | ||
| fn test_date_bin_compute_distance_i64_min() { |
There was a problem hiding this comment.
The unit test coverage here looks good for the UDF invocation path. Since this is SQL-visible behavior and there are already date_bin SLTs in datafusion/sqllogictest/test_files/datetime/timestamps.slt, it might be worth adding an end-to-end SLT case as well.
That would help verify the problematic scalar input returns NULL instead of panicking through the SQL execution path too.
|
Switched the modulo to checked_rem in 9afd01b so the i64::MIN % -1 case errors out cleanly, plus added a regression test (date_bin_nanos_interval(-1, i64::MIN, 0)). |
2 participants
Which issue does this PR close?
Rationale for this change
date_binpanics withattempt to subtract with overflowwhen the source timestamp sits neari64::MINbecausecompute_distancedoestime_diff - (time_diff % stride)and thentime_delta - strideon rawi64. The scalar pipeline already mapsErrfrombin_fnintoNULL, so the fix is to surface the overflow as a normal error.What changes are included in this PR?
compute_distanceto returnResult<i64>and usechecked_sub.date_bin_nanos_intervalanddate_bin_months_interval, plus replace the trailingorigin + time_deltawithchecked_add.Are these changes tested?
Yes. Added
test_date_bin_compute_distance_i64_minwhich previously panicked and now returnsNULL. Rancargo test -p datafusion-functions --lib -- datetime::date_bin,cargo fmt --check, andcargo clippy -p datafusion-functions --lib --tests --no-deps.Are there any user-facing changes?
Queries that previously panicked on extreme timestamps now return
NULL(consistent with the existing out-of-range behavior covered bytest_date_bin_out_of_range).